Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor!: split r/demo/users #1433

Merged
merged 16 commits into from
Feb 29, 2024

Conversation

harry-hov
Copy link
Contributor

@harry-hov harry-hov commented Dec 12, 2023

Splits r/demo/users into p/demo/users and r/demo/users

Related #1393

Contributors' checklist...
  • Added new tests, or not needed, or not feasible
  • Provided an example (e.g. screenshot) to aid review or the PR is self-explanatory
  • Updated the official documentation or not needed
  • No breaking changes were made, or a BREAKING CHANGE: xxx message was included in the description
  • Added references to related issues and PRs
  • Provided any useful hints for running manual tests
  • Added new benchmarks to generated graphs, if any. More info here.

@github-actions github-actions bot added the 🧾 package/realm Tag used for new Realms or Packages. label Dec 12, 2023
Copy link

codecov bot commented Dec 12, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 44.60%. Comparing base (75539ea) to head (362b39f).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1433      +/-   ##
==========================================
- Coverage   47.23%   44.60%   -2.64%     
==========================================
  Files         363      460      +97     
  Lines       59493    67872    +8379     
==========================================
+ Hits        28104    30272    +2168     
- Misses      29066    35067    +6001     
- Partials     2323     2533     +210     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@harry-hov harry-hov force-pushed the hariom/split-users-realm branch from 44ba3f6 to 793aa89 Compare January 14, 2024 23:09
@harry-hov harry-hov changed the title feat: split r/demo/users refactor: split r/demo/users Jan 14, 2024
@harry-hov harry-hov changed the title refactor: split r/demo/users refactor!: split r/demo/users Jan 14, 2024
@github-actions github-actions bot added the 📦 ⛰️ gno.land Issues or PRs gno.land package related label Jan 14, 2024
@harry-hov harry-hov marked this pull request as ready for review January 15, 2024 14:40
@harry-hov harry-hov requested a review from a team as a code owner January 15, 2024 14:40
Copy link
Contributor

@deelawn deelawn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is a great idea. But I also think this can be an opportunity to take this a bit further and separate additional application logic from what we might expect to be a part of a general package any realm can use to manage users.

examples/gno.land/p/demo/users/users.gno Show resolved Hide resolved
type User struct {
Address std.Address
Name string
Profile string
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are fields like profile, number, invites, and inviter needed as part of a basic User definition? These seem to be application specific. Or at the very least I should be able to write an application that can couple a user's address with its username without the extra fields I don't care about; that seems to be the base functionality.

But it also might be helpful to provide a struct like UserData or Extended user that provides the extra fields for like applications that want to operate using this data model:

type ExtendedUser struct {
    User
    Profile string
    Number int
    Invites int
    Inviter std.Address
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@deelawn agreed, I think it makes sense to keep those fields relevant to r/demo/users (when rendering pages and using its inviting system) so that third party realms/packages only get what they need most of the time (a name associated to an address)

@@ -1,7 +1,5 @@
package users

import "std"

type AddressOrName string

func (aon AddressOrName) IsName() bool {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These two methods also seem very application specific. I don't think there is any obvious reason to prefix usernames with @ in general. Though I do think this type can still be helpful to differentiate a string between being an address or name. This might be a good opportunity to make each method accept a usernamePrefix string argument that allows the application to decide this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@deelawn I think the way it currently is makes a lot of sense!

r/demo/users is, to me, the "username" realm every other realm should use to identify users. p/demo/users is not meant to be the "universal" implementation for all user systems.

If we have a realm function which is, say, the following:

func SendMessage(dst users.AddressOrName, title, content string)

having users.AddressOrName as the type indicates that, unless the realm's doing something weird, the function will accept a full address or also a username prefixed with @. So I'm personally in favour to keep it this way; p/demo/users is allowed to have some opinions :)

//----------------------------------------
// State

var (
admin std.Address = "g1us8428u2a5satrlxzagqqa5m6vmuze025anjlj"
name2User avl.Tree // Name -> *User
addr2User avl.Tree // std.Address -> *User
name2User avl.Tree // Name -> *users.User
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A user storage structure seems like a great candidate for something that should be included in a users package. It makes it easy for applications to maintain and access their own user list. Something like:

type p/users.Storage struct {
    nameToUser *avl.Tree
    addresstoUser *avl.Tree
}

func (s *Storage) GetUserByName(name string) *User
func (s *Storage) GetExtendeUserByName(name string) *ExtendedUser
func (s *Storage) getUserByName(name string) interface{}{ return s.nameToUser.Get(name) }

etc
...

@harry-hov harry-hov requested a review from a team as a code owner February 15, 2024 13:27
@harry-hov
Copy link
Contributor Author

@deelawn I love your suggestions 💯 However, I would like to stick to refactoring for this PR. I prefer not to introduce any changes to the logic. The main motivation behind this PR is to unblock PR #1393. Once this PR is merged, I will sum up your opinions in a separate issue. WDYT?

@deelawn
Copy link
Contributor

deelawn commented Feb 15, 2024

@deelawn I love your suggestions 💯 However, I would like to stick to refactoring for this PR. I prefer not to introduce any changes to the logic. The main motivation behind this PR is to unblock PR #1393. Once this PR is merged, I will sum up your opinions in a separate issue. WDYT?

Yeah fair enough 👍

@zivkovicmilos zivkovicmilos self-requested a review February 19, 2024 10:30
Copy link
Member

@thehowl thehowl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there's a "guideline" decision to be made here and to place on Effective Gno. Aside from that, the split looks good so pre-approving pending deciding what to do. (Please, in the meantime, do change the method where I commented from String to Render, as it was originally)

examples/gno.land/p/demo/microblog/microblog.gno Outdated Show resolved Hide resolved
@@ -1,7 +1,5 @@
package users

import "std"

type AddressOrName string

func (aon AddressOrName) IsName() bool {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@deelawn I think the way it currently is makes a lot of sense!

r/demo/users is, to me, the "username" realm every other realm should use to identify users. p/demo/users is not meant to be the "universal" implementation for all user systems.

If we have a realm function which is, say, the following:

func SendMessage(dst users.AddressOrName, title, content string)

having users.AddressOrName as the type indicates that, unless the realm's doing something weird, the function will accept a full address or also a username prefixed with @. So I'm personally in favour to keep it this way; p/demo/users is allowed to have some opinions :)

examples/gno.land/p/demo/users/users.gno Outdated Show resolved Hide resolved
type User struct {
Address std.Address
Name string
Profile string
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@deelawn agreed, I think it makes sense to keep those fields relevant to r/demo/users (when rendering pages and using its inviting system) so that third party realms/packages only get what they need most of the time (a name associated to an address)

@thehowl thehowl merged commit d32b583 into gnolang:master Feb 29, 2024
190 checks passed
@jefft0
Copy link
Contributor

jefft0 commented Mar 1, 2024

The description has the checked item for "No breaking changes were made, or a BREAKING CHANGE: xxx message was included in the description" . But the API has a breaking change, removing the getter methods. We will update GnoSocial to use the field without the getter.

func (u User) Name() string         { return u.name }
func (u User) Profile() string      { return u.profile }
func (u User) Address() std.Address { return u.address }

leohhhn pushed a commit to leohhhn/gno that referenced this pull request Mar 6, 2024
Splits `r/demo/users` into `p/demo/users` and `r/demo/users` 

Related gnolang#1393 

<details><summary>Contributors' checklist...</summary>

- [x] Added new tests, or not needed, or not feasible
- [x] Provided an example (e.g. screenshot) to aid review or the PR is
self-explanatory
- [x] Updated the official documentation or not needed
- [x] No breaking changes were made, or a `BREAKING CHANGE: xxx` message
was included in the description
- [x] Added references to related issues and PRs
- [x] Provided any useful hints for running manual tests
- [x] Added new benchmarks to [generated
graphs](https://gnoland.github.io/benchmarks), if any. More info
[here](https://github.com/gnolang/gno/blob/master/.benchmarks/README.md).
</details>
@jaekwon
Copy link
Contributor

jaekwon commented Mar 25, 2024

Thank you Hariom!

leohhhn pushed a commit that referenced this pull request Apr 17, 2024
### !!! BREAKING CHANGE: data type for `wugnot` public function's
parameter has been changed

Just like `foo20` respects a new `p/demo/users`, fix `wugnot` to respect
it too.

Related #1433 by @harry-hov 

<!-- please provide a detailed description of the changes made in this
pull request. -->

<details><summary>Contributors' checklist...</summary>

- [x] Added new tests, or not needed, or not feasible
- [ ] Provided an example (e.g. screenshot) to aid review or the PR is
self-explanatory
- [x] Updated the official documentation or not needed
- [x] No breaking changes were made, or a `BREAKING CHANGE: xxx` message
was included in the description
- [x] Added references to related issues and PRs
- [ ] Provided any useful hints for running manual tests
- [ ] Added new benchmarks to [generated
graphs](https://gnoland.github.io/benchmarks), if any. More info
[here](https://github.com/gnolang/gno/blob/master/.benchmarks/README.md).
</details>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📦 ⛰️ gno.land Issues or PRs gno.land package related 🧾 package/realm Tag used for new Realms or Packages.
Projects
Status: Done
Archived in project
Development

Successfully merging this pull request may close these issues.

6 participants